fix: resolve correctness, security, and resource bugs across subsystems#87
fix: resolve correctness, security, and resource bugs across subsystems#87leszek3737 wants to merge 1 commit into
Conversation
Fixes surfaced by a multi-agent audit + bug-hunt pass, each verified against the real code. Build clean, clippy zero-warning, all tests pass. Archives: - tar.zst creation finalizes the zstd frame (auto_finish); previously every created archive was silently truncated/corrupt - tar/zip/7z extraction re-verifies each entry's parent (drop the parent-dir cache that let a symlink swap escape the destination) - zip extraction fails loudly instead of silently truncating at 100k - zip creation enforces the same entry-count limit as tar - tar.xz decompression is size-capped (decompression-bomb guard) and no longer leaks its temp file on a reopen error File ops: - cross-device overwrite move no longer destroys the destination on a post-copy cancel (a completed copy is the point of no return) - files/symlinks get the critical-directory guard in the cross-device move fallback and the top-level symlink delete path - rename refuses to clobber a dangling symlink (symlink_metadata, not try_exists) - batch delete keeps hardlinked siblings (dedup on path, not inode) - single-owner temp cleanup removes the double-unlink + misleading log Jobs / event loop: - RunningJob::shutdown drains the bounded channel while joining, so a worker blocked on send() can no longer deadlock teardown - watcher debounced events flush every loop tick; event::poll errors shut the job down symmetrically with event::read - drop the always-overwritten final-progress dialog fs / watcher: - UID/GID name cache releases the global lock across NSS lookups - inotify rename pairing no longer steals an unrelated From on an unmatched cookie - deleted-symlink watcher state is reclaimed via the path cache Input / UI / viewer: - mouse double-click works again (Up no longer clears last_click) - page_down uses saturating_add; visual-offset cache guards overflow - hex-search highlight spans the 8-byte group gap correctly - input dialogs clear stale status on success; dialog toggle dedup'd Misc: - panel history uses VecDeque (O(1) eviction) - show_hidden config default consistent for missing table vs field - MTIME_TOLERANCE widened for FAT32 boundary rounding - PIPE_JOIN_TIMEOUT matched to CHAFA_TIMEOUT - CompareMode::ALL completeness test; tar.zst roundtrip + rename guard regression tests
Reviewer's GuideThis PR fixes multiple correctness, safety, and resource-handling bugs across archives, file operations, jobs/event loop, fs watcher, input/UI, and configuration, and adds tests and contributor/docs scaffolding to keep these invariants enforced. Sequence diagram for RunningJob shutdown with bounded progress channelsequenceDiagram
participant App
participant RunningJob
participant WorkerThread
participant ProgressChannel
App->>RunningJob: shutdown()
RunningJob->>RunningJob: cancel.store(true)
RunningJob->>WorkerThread: take(handle)
loop until REAPER_JOIN_DEADLINE
WorkerThread-->>RunningJob: is_finished()
alt not finished
RunningJob->>ProgressChannel: receiver.try_recv()
RunningJob->>RunningJob: sleep(REAPER_POLL_INTERVAL)
else finished
RunningJob->>RunningJob: break
end
end
alt handle finished
RunningJob->>WorkerThread: join()
WorkerThread-->>RunningJob: Result
else deadline exceeded
RunningJob->>RunningJob: log "detaching"
end
RunningJob-->>App: return
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive project documentation, issue templates, and robust safety and performance enhancements across file operations, archiving, and the terminal UI. Key improvements include resolving potential deadlocks in background jobs, preventing TOCTOU symlink vulnerabilities during archive extraction, optimizing history tracking with VecDeque, and hardening file operations against critical directory unlinking and clobbering. Feedback from the review highlights two critical issues: an artificial limit on zip extraction that should be removed for consistency, and a potential infinite loop leading to 100% CPU usage when visual cache invalidation occurs during a usize overflow.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Fail loudly instead of silently truncating: `.min(MAX_LIST_ENTRIES)` on the | ||
| // loop would extract only the first 100k entries yet still return `Ok`, | ||
| // reporting success on an incomplete extraction. The cap still guards against | ||
| // pathological entry counts — it just can no longer be mistaken for success. | ||
| if entry_count > MAX_LIST_ENTRIES { | ||
| return Err(ArchiveError::InvalidArchive(format!( | ||
| "archive has {entry_count} entries, exceeding the maximum {MAX_LIST_ENTRIES}" | ||
| ))); | ||
| } | ||
| extracted_paths.reserve(entry_count); |
There was a problem hiding this comment.
Limiting zip extraction to MAX_LIST_ENTRIES (100,000) and failing loudly when exceeded is inconsistent with tar and 7z extraction, which have no such limit. MAX_LIST_ENTRIES is intended to limit the number of entries shown in the TUI listing to keep it responsive, but extraction should be able to handle archives of any size (e.g., large node_modules or datasets). Removing this artificial limit entirely allows zip extraction to behave consistently with other formats.
| // Fail loudly instead of silently truncating: `.min(MAX_LIST_ENTRIES)` on the | |
| // loop would extract only the first 100k entries yet still return `Ok`, | |
| // reporting success on an incomplete extraction. The cap still guards against | |
| // pathological entry counts — it just can no longer be mistaken for success. | |
| if entry_count > MAX_LIST_ENTRIES { | |
| return Err(ArchiveError::InvalidArchive(format!( | |
| "archive has {entry_count} entries, exceeding the maximum {MAX_LIST_ENTRIES}" | |
| ))); | |
| } | |
| extracted_paths.reserve(entry_count); | |
| extracted_paths.reserve(entry_count); |
| self.invalidate_visual_cache(); | ||
| return; |
There was a problem hiding this comment.
When wrapping fails due to usize overflow, invalidate_visual_cache() is called, which resets cached_content_width to 0. Because visual_heights is empty and cached_content_width is 0, the next frame's call to update_wrap_layout will attempt to recalculate the layout again, leading to an infinite loop of O(N) recalculations on every frame and causing 100% CPU usage.\n\nTo prevent this, we should set cached_content_width to content_width even on failure, and update the early-return check at the top of update_wrap_layout to only check cached_content_width == content_width (removing the !visual_heights.is_empty() requirement, as a static file's layout won't change unless explicitly invalidated).
self.invalidate_visual_cache();\n *self.render_cache.cached_content_width.borrow_mut() = content_width;\n return;
Greptile SummaryThis is a comprehensive correctness and security bugfix PR addressing 25+ distinct issues found by a multi-agent audit. Fixes range from silent archive corruption (missing
Confidence Score: 4/5The PR is safe to merge. All substantive changes are targeted bugfixes with accompanying regression tests, and the correctness of each fix follows from its stated precondition. No change introduces new mutable shared state or changes a public API in a breaking way. The overwhelming majority of changes are clearly correct and well-tested. The two items worth a second look are: (1) CappedWriter::write increments its byte counter before the underlying write, so a partial write causes a small overcount—harmless for a 256 GiB bomb guard but imprecise; and (2) zip extraction now hard-rejects archives with >100k entries while tar extraction has no equivalent entry-count gate, which may surprise users of very large tar archives who see them listed but not extractable in zip form. Neither is a blocking concern for merge. src/ops/archive/tar.rs (CappedWriter write ordering) and src/ops/archive/zip.rs (entry limit asymmetry with tar extraction) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Archive["Archive ops (tar/zip/7z)"]
A1["create tar.zst\nauto_finish() fix"]
A2["extract tar/zip/7z\nre-verify parent every entry\n(no last_parent cache)"]
A3["zip extraction\nreject >100k entries loudly"]
A4["tar.xz decompress\nCappedWriter bomb guard"]
A5["tar.xz temp file\ncleanup on reopen error"]
end
subgraph FileOps["File operations"]
F1["cross-device overwrite move\nskip rollback after copy completes"]
F2["move/delete\ncritical-dir guard\nfor files & symlinks"]
F3["rename\nsymlink_metadata vs try_exists\n(dangling symlink protection)"]
F4["batch delete\npath dedup not inode dedup\n(keep hardlink siblings)"]
F5["chunk_copy publish_temp\nremove double-unlink"]
end
subgraph Jobs["Jobs / event loop"]
J1["RunningJob::shutdown\ndrain bounded channel\n(prevent deadlock)"]
J2["watcher flush_pending\nevery loop tick\n(not only on event)"]
J3["event::poll errors\nshutdown job + propagate"]
J4["poll_running_job\ndrop final-progress dialog\n(always overwritten)"]
end
subgraph FSWatcher["FS / watcher"]
W1["UID/GID cache\nrelease lock across NSS"]
W2["inotify rename cookie\nno unrelated From theft"]
W3["deleted symlink\nreclaim via path_cache"]
end
subgraph InputUI["Input / UI / viewer"]
U1["mouse double-click\nDon't clear last_click on Up"]
U2["page_down\nsaturating_add"]
U3["hex-search highlight\n8-byte gap fix"]
U4["input dialogs\nclear stale status on success"]
U5["visual-offset cache\nchecked_add overflow guard"]
end
subgraph Misc["Config / misc"]
M1["PersistedPanel Default\nshow_hidden=true"]
M2["MTIME_TOLERANCE\n2s→2500ms FAT32 rounding"]
M3["PIPE_JOIN_TIMEOUT\n=CHAFA_TIMEOUT"]
M4["panel history\nVecDeque O(1) eviction"]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph Archive["Archive ops (tar/zip/7z)"]
A1["create tar.zst\nauto_finish() fix"]
A2["extract tar/zip/7z\nre-verify parent every entry\n(no last_parent cache)"]
A3["zip extraction\nreject >100k entries loudly"]
A4["tar.xz decompress\nCappedWriter bomb guard"]
A5["tar.xz temp file\ncleanup on reopen error"]
end
subgraph FileOps["File operations"]
F1["cross-device overwrite move\nskip rollback after copy completes"]
F2["move/delete\ncritical-dir guard\nfor files & symlinks"]
F3["rename\nsymlink_metadata vs try_exists\n(dangling symlink protection)"]
F4["batch delete\npath dedup not inode dedup\n(keep hardlink siblings)"]
F5["chunk_copy publish_temp\nremove double-unlink"]
end
subgraph Jobs["Jobs / event loop"]
J1["RunningJob::shutdown\ndrain bounded channel\n(prevent deadlock)"]
J2["watcher flush_pending\nevery loop tick\n(not only on event)"]
J3["event::poll errors\nshutdown job + propagate"]
J4["poll_running_job\ndrop final-progress dialog\n(always overwritten)"]
end
subgraph FSWatcher["FS / watcher"]
W1["UID/GID cache\nrelease lock across NSS"]
W2["inotify rename cookie\nno unrelated From theft"]
W3["deleted symlink\nreclaim via path_cache"]
end
subgraph InputUI["Input / UI / viewer"]
U1["mouse double-click\nDon't clear last_click on Up"]
U2["page_down\nsaturating_add"]
U3["hex-search highlight\n8-byte gap fix"]
U4["input dialogs\nclear stale status on success"]
U5["visual-offset cache\nchecked_add overflow guard"]
end
subgraph Misc["Config / misc"]
M1["PersistedPanel Default\nshow_hidden=true"]
M2["MTIME_TOLERANCE\n2s→2500ms FAT32 rounding"]
M3["PIPE_JOIN_TIMEOUT\n=CHAFA_TIMEOUT"]
M4["panel history\nVecDeque O(1) eviction"]
end
Reviews (1): Last reviewed commit: "fix: resolve correctness, security, and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl<W: Write> Write for CappedWriter<W> { | ||
| fn write(&mut self, buf: &[u8]) -> io::Result<usize> { | ||
| self.written = self.written.saturating_add(buf.len() as u64); | ||
| if self.written > super::MAX_TOTAL_ARCHIVE_SIZE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "decompressed xz exceeds maximum archive size", | ||
| )); | ||
| } | ||
| self.inner.write(buf) | ||
| } |
There was a problem hiding this comment.
written counter is incremented by the full buf.len() before self.inner.write(buf) is called. If the inner writer does a short write (returns Ok(n) where n < buf.len()), the counter overcounts by buf.len() - n for that call. When the caller retries with the remaining bytes, those are counted again, so one logical chunk of buf.len() bytes ends up counted as roughly 2 * buf.len() - n. For a 256 GiB cap written to a File this almost never triggers (kernel writes to temp files are rarely short), but the standard, precise pattern is to count the bytes actually committed by the inner writer.
| impl<W: Write> Write for CappedWriter<W> { | |
| fn write(&mut self, buf: &[u8]) -> io::Result<usize> { | |
| self.written = self.written.saturating_add(buf.len() as u64); | |
| if self.written > super::MAX_TOTAL_ARCHIVE_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "decompressed xz exceeds maximum archive size", | |
| )); | |
| } | |
| self.inner.write(buf) | |
| } | |
| impl<W: Write> Write for CappedWriter<W> { | |
| fn write(&mut self, buf: &[u8]) -> io::Result<usize> { | |
| let n = self.inner.write(buf)?; | |
| self.written = self.written.saturating_add(n as u64); | |
| if self.written > super::MAX_TOTAL_ARCHIVE_SIZE { | |
| return Err(io::Error::new( | |
| io::ErrorKind::InvalidData, | |
| "decompressed xz exceeds maximum archive size", | |
| )); | |
| } | |
| Ok(n) | |
| } |
| // Fail loudly instead of silently truncating: `.min(MAX_LIST_ENTRIES)` on the | ||
| // loop would extract only the first 100k entries yet still return `Ok`, | ||
| // reporting success on an incomplete extraction. The cap still guards against | ||
| // pathological entry counts — it just can no longer be mistaken for success. | ||
| if entry_count > MAX_LIST_ENTRIES { | ||
| return Err(ArchiveError::InvalidArchive(format!( | ||
| "archive has {entry_count} entries, exceeding the maximum {MAX_LIST_ENTRIES}" | ||
| ))); | ||
| } | ||
| extracted_paths.reserve(entry_count); |
There was a problem hiding this comment.
extract_zip_entries now hard-rejects archives with more than MAX_LIST_ENTRIES (100k) entries, but extract_tar has no equivalent entry-count guard on the extraction path (only the TotalSizeGuard byte cap). A user who opens a tar with 150k files will see it listed (truncated at 100k by take(MAX_LIST_ENTRIES) in list_tar) and may attempt extraction without realising zip would fail at the same count. Applying the same entry-count check to extract_tar, or at least documenting the intentional asymmetry, would keep the two formats consistent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
I now have a comprehensive understanding of this PR. Here's my review. PR Review: fix: resolve correctness, security, and resource bugs across subsystemsVerification StatusAll CI gates pass:
Overall AssessmentThis is a high-quality PR. The fixes are well-reasoned, each with thorough comments explaining the why. The security hardening (TOCTOU parent re-verification, decompression-bomb guards, critical-directory guards, dangling-symlink rename protection) is sound, and the regression tests are meaningful. Issues Found1. Missing test for the
The correctness-critical assertion — "a post-copy cancel does NOT delete the destination when 2.
3. Sevenz extraction parent re-verification is O(n) canonicalize per entry (low/perf)
4. Zip extraction is now a hard failure above 100k entries (behavioral change)
Things Checked and Found Correct
Non-Code Observations
RecommendationApprove with one suggestion: add a test for the |
1 participant
Fixes surfaced by a multi-agent audit + bug-hunt pass, each verified against the real code. Build clean, clippy zero-warning, all tests pass.
Archives:
File ops:
Jobs / event loop:
fs / watcher:
Input / UI / viewer:
Misc:
Summary by Sourcery
Resolve multiple correctness, safety, and UX issues across archives, file ops, jobs, watcher, viewer, and input, and add contributor and issue templates.
New Features:
Bug Fixes:
..filenames render non-empty.Enhancements:
Tests: